Skip to content

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 7, 2025

CLDR-18464

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

CC @roozbehp @pedberg-icu @AEApple

<calendar type="islamic">
<calendarSystem type="lunar" />
<eras>
<era type="-1" end="622-7-14" code="islamic-inverse" aliases="bh"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it the code "islamic-inverse" because I want to land the code updates, #4519, atomically once we have agreement on them.

<calendar type="islamic-civil">
<calendarSystem type="lunar" />
<eras>
<era type="-1" end="622-7-15" code="islamic-civil-inverse" aliases="bh"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it the type -1 because type 0 is already used for the AH era. Please let me know if I should instead:

  1. Use a positive number like 1 (and leave AH as 0)
  2. Change BH to 0, AH to 1, and update all the XML files that currently have translations for AH at type 0 to instead be at type 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sffc Usually this number is used as an array index in ICU. I guess I would be in favor of your option #2 above. We would need to add a migration note for ICU (and for CLDR), and probably update some ICU code. I think we allow years to be negative, but I do not think we allow eras to be.

@sffc sffc removed their assignment Apr 7, 2025
@sffc sffc requested a review from AEApple April 7, 2025 20:44
Copy link
Contributor

@roozbehp roozbehp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me. I would also prefer the "Change BH to 0, AH to 1" option, as it seems cleanest.

@sffc
Copy link
Member Author

sffc commented Apr 10, 2025

This means changing all the XML files, which I'm willing to do and can probably automate, but to make sure, before I proceed, is it sufficient for me to change something like

					<eraAbbr>
						<era type="0">যুগ</era>
					</eraAbbr>

to

					<eraAbbr>
						<era type="1">যুগ</era>
					</eraAbbr>

without adding structure for type="0"? It will inherit automatically, right?

@pedberg-icu
Copy link
Contributor

This means changing all the XML files, which I'm willing to do and can probably automate...It will inherit automatically, right?

As far as I know, yes.

@sffc
Copy link
Member Author

sffc commented Apr 11, 2025

Does this look right?

Here's how I generated the diff:

$ xmlstarlet ed --inplace -P -u "/ldml/dates/calendars/calendar[@type='islamic']/eras/*/era[@type=0]/@type" -v 1 *.xml

That updated all the XML files, but it also resulted in some other unwanted diffs to the XML. I undid those by manually touching up the diff file to remove diffs on lines containing &quot. I then went through the remaining changes and verified that they were touching only the era IDs.

To verify that I did everything, correct, I then ran:

$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic']/eras/*/era[@type=0]" *.xml
Warning: program compiled against libxml 212 using older 209
Before Hijrah
BH
BH
$

So it found the three BH entries I put in and nothing else.

@sffc
Copy link
Member Author

sffc commented Apr 11, 2025

The other calendars don't have data. Only islamic.

$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-civil']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-rgsa']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-umalqura']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-tbla']/eras/*/era[@type=0]" *.xml
$ 

@AEApple AEApple requested a review from srl295 April 12, 2025 00:46
srl295
srl295 previously approved these changes Apr 15, 2025
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs commit fix

@sffc
Copy link
Member Author

sffc commented Apr 16, 2025

@srl295, do you prefer me to merge the PR in this shape (AH becomes index 1, BH inserted at index 0) or would you prefer keeping AH at index 0 and adding BH at index 1 as @FrankYFTang suggested?

@pedberg-icu
Copy link
Contributor

@srl295, do you prefer me to merge the PR in this shape (AH becomes index 1, BH inserted at index 0) or would you prefer keeping AH at index 0 and adding BH at index 1 as @FrankYFTang suggested?

@sffc I am not @srl295 but I now prefer keeping AH at index 0 and adding BH at index 1

@srl295 srl295 self-requested a review April 16, 2025 21:23
@sffc
Copy link
Member Author

sffc commented Apr 17, 2025

OK please re-review.

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall PR is straightforward to review now, with changes in other locales undone

@srl295
Copy link
Member

srl295 commented Apr 18, 2025

Do we need a followup issue to make sure CLDR spec clarifies this question so that it doesn't need to get discussed again? Variants and/or lack thereof?

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member Author

sffc commented Apr 18, 2025

This is ready to merge; should I Squash And Merge or Rebase And Merge?

I'd like to use CLDR-18369 for all spec updates on this topic.

@pedberg-icu
Copy link
Contributor

This is ready to merge; should I Squash And Merge or Rebase And Merge?

We usually Squash and merge except for special circumstances such as cherry-picking maint branch changes to main. But since this only has 1 commit does it make a difference? Does squash gives a different commit hash?

@sffc sffc merged commit 632c93a into unicode-org:main Apr 19, 2025
9 checks passed
@sffc sffc deleted the CLDR-18464-bh branch April 19, 2025 05:55
@pedberg-icu
Copy link
Contributor

This causes an IndexOutOfBoundsException at org.unicode.cldr.test.ExampleGenerator.handleEras(ExampleGenerator.java:3158), because there is a reference to a hard-coded CALENDAR_ERAS HashMap maps "islamic" to a sample date for only a single era, need to add another entry for the new islamic era. I will make a PR to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants